Skip to content

feat: initial streams support and convert to I/O module #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

grabbou
Copy link
Collaborator

@grabbou grabbou commented Nov 6, 2024

This is just an experiment and a bit naive implementation. Needs some fine tuning, but it's a good proof of concept of a readable stream (that is coming from the native side).

What it does?

  • Create stream on both Native and JavaScript side
  • Modify/transform streams on the JavaScript side
  • Pass it back to Native

This API is split across two parts:

  • Built-in native API
  • DOM compatibility API (that uses Readable/Writable shim)

@grabbou
Copy link
Collaborator Author

grabbou commented Nov 6, 2024

Need to rename this package to i/o module where WebSockets is going to be just one part of it.

@grabbou grabbou changed the title feat: initial streams support feat: initial streams support and convert to I/O module Nov 7, 2024
Copy link

@robik robik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@grabbou
Copy link
Collaborator Author

grabbou commented Nov 7, 2024

@robik please see latest one (but disregard StreamMangaer, it needs some work)

@grabbou grabbou requested a review from mani3xis November 7, 2024 11:34
Copy link

@mani3xis mani3xis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 💪
I just left some questions and a few nitpicks


async write(chunk: ArrayBuffer) {
if (!outputStream.hasSpaceAvailable()) {
throw new Error('No space available in output stream')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Q] Do we really have to throw here? Cannot we apply backpressure or wait for the space to become available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds like we should handle this! Not sure how easy/hard would it be!


export interface InputStream extends HybridObject<{ ios: 'swift' }> {
hasBytesAvailable(): boolean
read(buffer: ArrayBuffer, maxLength: number): number
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] This would be a pulling API, right? Maybe we can return a Promise here to get notified that data is available, or a callback? wdyt?
[nit] How do you distinguish EOF and empty buffer (when used in a non-blocking way)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good questions. Please check https://developer.apple.com/documentation/foundation/inputstream and let me know the answer! 🥲

@grabbou
Copy link
Collaborator Author

grabbou commented Nov 8, 2024

Thank you @mani3xis for your review. I have addressed some of your comments (answered & resolved), please feel free to unresolve ro follow-up if another issue persist.

The remaining comments are great next-steps and we should convert them to issues and just get done!

NOTE: This PR does not implement Android, so after merging it will be temporarily broken which is fine 🤣

@grabbou
Copy link
Collaborator Author

grabbou commented Nov 8, 2024

Cleaned up history of the PR for rebase/merge instead of a squash.

@grabbou grabbou merged commit cde0bb0 into main Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants